Stop fallback to XPath.match in FunctionsClass#send - #364
Merged
Conversation
Return value of xpath function should be either number, string, boolean or nodeset. But the fallback path returns `XPath.match(unregistered_function_name)` which may return number/string/boolean wrapped in an array. Instead of rejecting these invalid value types or unwrapping the array, simply removing the XPath-noncompliant behavior is better.
There was a problem hiding this comment.
Pull request overview
This pull request removes the non-standard fallback behavior where an unregistered XPath function call could implicitly be treated as an element lookup via XPath.match, which could also contaminate return types (e.g., arrays wrapping scalars). The change aligns function evaluation more closely with XPath expectations by returning an empty nodeset for unknown functions.
Changes:
- Update
FunctionsClass#sendto return[]for unregistered functions instead of falling back toXPath.match(...). - Adjust function tests to assert
nil/empty results for unknown functions and add coverage for previously problematic cases (e.g.,"42()").
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lib/rexml/functions.rb | Removes XPath.match fallback for unknown functions and returns an empty nodeset instead. |
| test/functions/test_base.rb | Updates expectations for unknown functions and adds assertions preventing invalid return type contamination. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
naitoh
approved these changes
Aug 31, 2026
Contributor
|
Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related to #342, I found one more contamination path.
Return value of xpath function should be either number, string, boolean or nodeset. But the fallback path returns
XPath.match(unregistered_function_name)which may return number/string/boolean wrapped in an array. (#353)Instead of rejecting these invalid value types or unwrapping the array, simply removing the XPath-
noncompliant behavior is better.
The TODO comment below is kept because falling back to
[]instead of raising error may be still not XPath spec behavior.Related bugs/glitch (though it's probably a combination of bug in REXML::Parsers::XPathParser)